patch(DPE-9339): MongoDB Encryption At Rest#258
Conversation
…ment-encryption-at-rest-according-to-approved-design
patriciareinoso
left a comment
There was a problem hiding this comment.
extremely quick pass over the code
…ment-encryption-at-rest-according-to-approved-design
783989d to
19fd804
Compare
Co-authored-by: Patricia Reinoso <patricia.reinoso@canonical.com> Signed-off-by: Neha Oudin <17551419+Gu1nness@users.noreply.github.com>
Co-authored-by: Patricia Reinoso <patricia.reinoso@canonical.com> Signed-off-by: Neha Oudin <17551419+Gu1nness@users.noreply.github.com>
Co-authored-by: Patricia Reinoso <patricia.reinoso@canonical.com> Signed-off-by: Neha Oudin <17551419+Gu1nness@users.noreply.github.com>
…ment-encryption-at-rest-according-to-approved-design
…ncryption-at-rest-implement-encryption-at-rest-according-to-approved-design
Mehdi-Bendriss
left a comment
There was a problem hiding this comment.
Thanks Neha, Great work! :)
I left a few comments, but my main concern is on upgrades and see how it all behaves if for ex the token expired (we'll need to followup with another PR for an integration test about it)
| self.relation_name = ExternalRequirerRelations.VAULT.value | ||
|
|
||
| super().__init__(parent=dependent, key=self.relation_name) | ||
| self.interface = vault_kv.VaultKvRequires(self.charm, self.relation_name, self.charm.name) |
There was a problem hiding this comment.
nit
| self.interface = vault_kv.VaultKvRequires(self.charm, self.relation_name, self.charm.name) | |
| self.kv_interface = vault_kv.VaultKvRequires(self.charm, self.relation_name, self.charm.name) |
| self.manager.set_status(VaultStatuses.INVALID_CONFIG.value, scope="both") | ||
| return | ||
|
|
||
| def _on_gone_away(self, event: vault_kv.VaultKvGoneAwayEvent) -> None: |
There was a problem hiding this comment.
I have 2 questions here,
- Did we test the flow when the Vault relation is removed and then re-established?
- What is the mongodb and vault agent behaviors regarding the expiration of the token?
| self.manager.set_status(VaultStatuses.VAULT_INTEGRATED.value, scope="both") | ||
| return | ||
| self.manager.clear_statuses(scope="both") | ||
| egress_subnets = self.manager.get_subnets() |
There was a problem hiding this comment.
probably worth renaming get_subnets to get_egress_subnets for clarity
| return | ||
| egress_subnets = self.manager.get_subnets() | ||
| nonce = self.manager.get_nonce() | ||
| self.interface.request_credentials( |
There was a problem hiding this comment.
Could you explain this?
| return template.render(**new_content) | ||
|
|
||
| @override | ||
| def set_environment(self): |
There was a problem hiding this comment.
missing return type hint and in configure_and_restart
There was a problem hiding this comment.
missing docstrings in tests, and could you add more inline comments to document the flow and expectations?
There was a problem hiding this comment.
missing docstrings in some helpers - and could you add more inline comments to explain some of the flows?
There was a problem hiding this comment.
Missing docstrings and some explanations inline comments here and there
|
|
||
| @pytest.mark.abort_on_fail | ||
| async def test_integration_goes_to_active( | ||
| ops_test: OpsTest, substrate: Substrate, vault_charm_name: str |
There was a problem hiding this comment.
can we add continuous writes on all tests from this one on? I'd like to see the write/read side of things and expectations on the data side for every Vault related operation.
| app_name, | ||
| status="blocked", | ||
| message="Must be integrated with vault to enable encryption at rest.", | ||
| ) |
There was a problem hiding this comment.
Can we add 2 more tests about the restoring of the relation:
- after just a quick time while the token is still alive
- after the token expiration time
and assert that the health is restored and data can be read / written
🏷️ Type of changes
📝 Description
This is the implementation of MongoDB encryption at rest.
It uses Vault (Vault-k8s on kubernetes) as a backend to store the encryption keys.
It uses a config option to enable encryption at rest at boot time.
It uses an action to rotate the master key.
🧪 Manual testing steps
🌞 Sunny test
🌧️ Rainy test:
🔬 Automated testing steps
Positive checks that:
Negative checks that:
✅ Checklist